feat: add CI/CD enhancements, test infrastructure, and tooling#255
feat: add CI/CD enhancements, test infrastructure, and tooling#255you615074-png wants to merge 3 commits into
Conversation
- Add pyproject.toml with ruff, mypy, pytest configuration - Add .pre-commit-config.yaml with linting and formatting hooks - Add Makefile with 12 targets for common project commands - Enhance CI workflow with 4 new jobs: python-syntax, typescript-syntax, quiz-coverage, and pytest matrix - Add scripts/quiz_coverage.py for per-phase quiz coverage reporting - Add scripts/scaffold_quizzes.py to generate quiz.json skeletons for 165+ lessons currently missing quizzes - Add scripts/scaffold_tests.py to generate pytest skeletons for lessons without tests (~470 lessons) - Add scripts/ts_syntax_check.mjs for TypeScript syntax checking without dependency resolution (handles hono/zod/anthropic imports gracefully) - Fix LESSON_TEMPLATE.md to match actual lesson structure: remove notebook/ directory, add quiz.json schema, add agent-*.md outputs, revise code comment guidelines - Add 45 concrete example tests: - Agent Loop (19 tests): ToyLLM, ToolRegistry, Calculator, KVStore, AgentLoop - Linear Algebra (26 tests): Vector, Matrix, is_independent, Gram-Schmidt Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR establishes the foundational infrastructure, standards, and developer tooling for the curriculum project. It introduces lesson standards (folder structure, quiz schema, test requirements), project tooling configuration (ruff, mypy, pytest via pyproject.toml and pre-commit hooks), automation scripts for scaffolding quizzes and tests plus syntax validation, Makefile targets for common developer commands, CI workflow advisory checks, and two example lesson test suites demonstrating the new standards in practice. ChangesCurriculum Infrastructure & Developer Tooling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
phases/01-math-foundations/01-linear-algebra-intuition/code/tests/test_main.py (1)
23-26: 💤 Low valueAdd
strict=Truetozip()for safer iteration.Line 25 uses
zip()without an explicitstrict=parameter. Since the test file requires Python 3.10+, you can addstrict=Trueto ensure the component lists have equal length (though they should by construction).♻️ Proposed change
def test_sub(self) -> None: v = vectors.Vector([5, 7, 9]) - vectors.Vector([1, 2, 3]) - for a, b in zip(v.components, [4.0, 5.0, 6.0]): + for a, b in zip(v.components, [4.0, 5.0, 6.0], strict=True): assert abs(a - b) < 1e-10🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phases/01-math-foundations/01-linear-algebra-intuition/code/tests/test_main.py` around lines 23 - 26, Update the zip invocation in the test_sub test to use strict=True to ensure both sequences are the same length; in function test_sub (which constructs v = vectors.Vector([5, 7, 9]) - vectors.Vector([1, 2, 3]) and iterates over v.components), change the zip(v.components, [4.0, 5.0, 6.0]) call to zip(v.components, [4.0, 5.0, 6.0], strict=True) so mismatched lengths raise an error rather than silently truncating.scripts/ts_syntax_check.mjs (1)
90-94: 💤 Low valueSimplify diagnostics assignment (optional).
Lines 91-93 use
[].concat()to wrap the result ofgetParseDiagnostics(), butgetParseDiagnostics()already returns an array. You can simplify to:const diagnostics = ts.getParseDiagnostics(sf);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ts_syntax_check.mjs` around lines 90 - 94, The diagnostics assignment currently wraps ts.getParseDiagnostics(sf) in [].concat() unnecessarily; update the diagnostics variable to directly use the array returned by ts.getParseDiagnostics(sf) (replace the [].concat(ts.getParseDiagnostics(sf)) expression with ts.getParseDiagnostics(sf)) so the code reads const diagnostics = ts.getParseDiagnostics(sf); and remove the redundant concat wrapper.LESSON_TEMPLATE.md (1)
165-177: ⚡ Quick winConsider adding the 5+ unit tests requirement.
The "Code File Guidelines" section documents header comments and inline comment policy, but doesn't mention the 5+ unit tests minimum that is enforced for lesson code. Based on learnings, lesson code must include at least 5 unit tests runnable via stdlib test runner.
📋 Suggested addition
## Code File Guidelines - Code must run without errors - Add a 4–6 line header comment citing the lesson path and any external spec/RFC referenced by the implementation - Use inline comments sparingly — let the code speak, but don't be dogmatic about "zero comments." Some algorithms need a one-liner to orient the reader - Use the language that fits best for the topic - Include a `# requires: pkg1, pkg2` comment at the top if your entry file needs packages outside the Python stdlib (see `scripts/lesson_run.py`) +- Include 5+ unit tests in `code/tests/`, runnable via the stdlib test runner + (`python3 -m unittest discover`, `npx tsx --test`, Rust/Julia inline) - Start simple, build up complexity - Every function and class should have a clear purposeBased on learnings, lesson code must include 5+ unit tests minimum, runnable via stdlib test runner.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@LESSON_TEMPLATE.md` around lines 165 - 177, Update the "Code File Guidelines" section to explicitly require "Include at least 5 unit tests" that are runnable via the Python stdlib test runner; specifically add a bullet saying lesson code must include 5+ unit tests in files named test_*.py (or discoverable by python -m unittest) and that tests should run with the project's runner (scripts/lesson_run.py) and not rely on external frameworks unless declared in a `# requires:` header. This clarifies the enforced minimum and how tests must be structured and executed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/curriculum.yml:
- Around line 81-94: The typescript-syntax job is labeled advisory but will fail
the workflow because the step running node scripts/ts_syntax_check.mjs exits
non-zero on errors; update the typescript-syntax job to be non-blocking by
either (A) replacing the single-step run of node scripts/ts_syntax_check.mjs in
the "syntax-check all .ts files" step with a small wrapper that captures the
JSON output from scripts/ts_syntax_check.mjs, parses it and prints warnings
(mirroring the python-syntax handling), and exits 0, or (B) simpler: set
continue-on-error: true on the "syntax-check all .ts files" step so failures are
advisory; modify the typescript-syntax job/step accordingly.
In @.pre-commit-config.yaml:
- Around line 31-38: The comment "mypy is advisory only — do not block commits"
is misleading because the pre-commit hook with id: mypy currently blocks commits
on failure; either remove that comment or make the hook non-blocking by adding
the advisory option (e.g., add verbose: true or set the hook to not fail) to the
mypy hook configuration; update the block that contains repo:
https://github.com/pre-commit/mirrors-mypy, rev: v1.15.0, hooks -> - id: mypy to
reflect the chosen change.
In `@Makefile`:
- Around line 41-42: The Makefile test target uses "--timeout=60" which
conflicts with pyproject.toml's "timeout = 30"; update the Makefile test target
(the "test:" rule invoking $(PYTHON) -m pytest --timeout=60 -x) to use the same
timeout value as pyproject.toml (30) or, if 60s is intentional, add a brief
comment above the "test:" target explaining why this override is required so
maintainers understand the discrepancy.
- Around line 70-72: The scaffold Makefile target currently only validates SLUG;
add validation for PHASE as well so both required variables are checked before
calling scaffold-lesson.sh: in the scaffold target (symbol: scaffold) ensure you
test -n "$(PHASE)" and test -n "$(SLUG)" and emit the usage message "Usage: make
scaffold PHASE=NN SLUG=lesson-name" and exit non‑zero if either is missing, then
invoke bash scripts/scaffold-lesson.sh $(PHASE) $(SLUG) only when both are
present.
In `@scripts/quiz_coverage.py`:
- Around line 148-152: The printed phase path reconstruction is lossy—stop
rebuilding the slug from c.phase_name; instead preserve and use the original
directory slug in PhaseCoverage. Update the code that builds PhaseCoverage
(e.g., constructor/factory for PhaseCoverage and the function that maps slugs to
names like _phase_slug_to_name) to store the original slug into a field (e.g.,
phase_slug or original_slug), and then change the printing loop (the block
iterating coverages and using c.phase_id and c.phase_name) to use that stored
slug (c.phase_slug) when formatting the output path so the exact original
directory name is emitted.
In `@scripts/scaffold_tests.py`:
- Around line 83-87: In the lines that call lines.append which build the comment
block (the three calls that currently use f-strings but contain no
placeholders), remove the unnecessary leading "f" so they become normal string
literals (keep the final lines.append that uses fname/rel/py_files as an
f-string since it contains placeholders); look for the lines.append(...) calls
that reference rel, fname and py_files to identify the correct statements and
change only the f-prefixed literals that have no interpolation.
---
Nitpick comments:
In `@LESSON_TEMPLATE.md`:
- Around line 165-177: Update the "Code File Guidelines" section to explicitly
require "Include at least 5 unit tests" that are runnable via the Python stdlib
test runner; specifically add a bullet saying lesson code must include 5+ unit
tests in files named test_*.py (or discoverable by python -m unittest) and that
tests should run with the project's runner (scripts/lesson_run.py) and not rely
on external frameworks unless declared in a `# requires:` header. This clarifies
the enforced minimum and how tests must be structured and executed.
In
`@phases/01-math-foundations/01-linear-algebra-intuition/code/tests/test_main.py`:
- Around line 23-26: Update the zip invocation in the test_sub test to use
strict=True to ensure both sequences are the same length; in function test_sub
(which constructs v = vectors.Vector([5, 7, 9]) - vectors.Vector([1, 2, 3]) and
iterates over v.components), change the zip(v.components, [4.0, 5.0, 6.0]) call
to zip(v.components, [4.0, 5.0, 6.0], strict=True) so mismatched lengths raise
an error rather than silently truncating.
In `@scripts/ts_syntax_check.mjs`:
- Around line 90-94: The diagnostics assignment currently wraps
ts.getParseDiagnostics(sf) in [].concat() unnecessarily; update the diagnostics
variable to directly use the array returned by ts.getParseDiagnostics(sf)
(replace the [].concat(ts.getParseDiagnostics(sf)) expression with
ts.getParseDiagnostics(sf)) so the code reads const diagnostics =
ts.getParseDiagnostics(sf); and remove the redundant concat wrapper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b2507312-4dd0-4ebb-a218-321db4cb3916
📒 Files selected for processing (11)
.github/workflows/curriculum.yml.pre-commit-config.yamlLESSON_TEMPLATE.mdMakefilephases/01-math-foundations/01-linear-algebra-intuition/code/tests/test_main.pyphases/14-agent-engineering/01-the-agent-loop/code/tests/test_main.pypyproject.tomlscripts/quiz_coverage.pyscripts/scaffold_quizzes.pyscripts/scaffold_tests.pyscripts/ts_syntax_check.mjs
…mplate - curriculum.yml: add continue-on-error to typescript-syntax job so it truly runs as advisory and never blocks the workflow - .pre-commit-config.yaml: fix misleading mypy comment (the hook does block; note the SKIP=mypy escape hatch) - Makefile: sync pytest timeout to 30s matching pyproject.toml - LESSON_TEMPLATE.md: add 5+ unit tests requirement to code guidelines Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.pre-commit-config.yaml:
- Line 38: The comment in .pre-commit-config.yaml incorrectly states "mypy
checks scripts/ with permissive rules"; update that comment to say scripts/ uses
stricter/strict mypy rules per pyproject.toml (see the [[tool.mypy.overrides]]
for scripts.* and the disallow_untyped_defs = true override) so the comment
accurately reflects that scripts/ is checked with stricter settings rather than
permissive ones.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 88d1cfdc-8c59-4583-8d04-6e2781539ea6
📒 Files selected for processing (4)
.github/workflows/curriculum.yml.pre-commit-config.yamlLESSON_TEMPLATE.mdMakefile
🚧 Files skipped from review as they are similar to previous changes (3)
- Makefile
- LESSON_TEMPLATE.md
- .github/workflows/curriculum.yml
- pre-commit: correct mypy comment — scripts/ uses stricter rules, not permissive - Makefile: validate PHASE alongside SLUG in scaffold target - ts_syntax_check.mjs: remove unnecessary [].concat() wrapper - scaffold_tests.py: drop no-op f-prefix on static string literals - quiz_coverage.py: store and use original phase_slug instead of lossy reconstruction - test_linear_algebra.py: add strict=True to zip() for safer iteration - Rename test_main.py to test_<slug>.py to avoid module-name collisions across phases - LESSON_TEMPLATE.md: recommend slug-based test file names for uniqueness Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@phases/01-math-foundations/01-linear-algebra-intuition/code/tests/test_linear_algebra.py`:
- Line 25: Confirm that using zip(..., strict=True) in test_linear_algebra.py is
safe by ensuring the project's Python requirement covers it (check
requires-python in pyproject.toml is >=3.10) and CI runs a Python >=3.8 (CI uses
3.12); if the project did not guarantee that, replace the expression in the test
that iterates v.components (the zip call with strict=True) with a compatible
alternative (e.g., explicit length check plus plain zip) so tests run on older
interpreters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3a7f1f8-f261-4b68-940d-d98c04ff5288
📒 Files selected for processing (8)
.pre-commit-config.yamlLESSON_TEMPLATE.mdMakefilephases/01-math-foundations/01-linear-algebra-intuition/code/tests/test_linear_algebra.pyphases/14-agent-engineering/01-the-agent-loop/code/tests/test_agent_loop.pyscripts/quiz_coverage.pyscripts/scaffold_tests.pyscripts/ts_syntax_check.mjs
💤 Files with no reviewable changes (1)
- phases/14-agent-engineering/01-the-agent-loop/code/tests/test_agent_loop.py
🚧 Files skipped from review as they are similar to previous changes (6)
- Makefile
- .pre-commit-config.yaml
- LESSON_TEMPLATE.md
- scripts/quiz_coverage.py
- scripts/scaffold_tests.py
- scripts/ts_syntax_check.mjs
|
|
||
| def test_sub(self) -> None: | ||
| v = vectors.Vector([5, 7, 9]) - vectors.Vector([1, 2, 3]) | ||
| for a, b in zip(v.components, [4.0, 5.0, 6.0], strict=True): |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check Python version requirements across the project
# Check pyproject.toml for requires-python
if [ -f pyproject.toml ]; then
echo "=== pyproject.toml requires-python ==="
rg -A1 'requires-python|python_requires' pyproject.toml
fi
# Check for .python-version
if [ -f .python-version ]; then
echo "=== .python-version ==="
cat .python-version
fi
# Check setup.py for python_requires
if [ -f setup.py ]; then
echo "=== setup.py python_requires ==="
rg 'python_requires' setup.py
fi
# Check CI workflow for Python version matrix
echo "=== CI workflow Python versions ==="
rg -A3 'python-version' .github/workflows/ || echo "No CI Python version config found"Repository: rohitg00/ai-engineering-from-scratch
Length of output: 1892
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for how tests are run (pytest vs unittest) ==="
rg -n "unittest\.discover|python3 -m unittest discover|pytest|npx tsx --test" -S .github/workflows scripts phases || true
echo
echo "=== Check lesson/test runner script(s) ==="
rg -n "pytest|unittest" scripts/lesson_run.py scripts/audit_lessons.py scripts/quiz_coverage.py 2>/dev/null || true
ls -la scripts 2>/dev/null | headRepository: rohitg00/ai-engineering-from-scratch
Length of output: 8499
Confirm strict=True compatibility with the project’s Python version
zip(..., strict=True) is safe for this repo: pyproject.toml declares requires-python = ">=3.10", and CI runs tests on Python 3.12.
Note: this test file is pytest-oriented (run instruction uses python -m pytest), so it may not satisfy any requirement to be runnable via python3 -m unittest discover specifically for this lesson suite.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@phases/01-math-foundations/01-linear-algebra-intuition/code/tests/test_linear_algebra.py`
at line 25, Confirm that using zip(..., strict=True) in test_linear_algebra.py
is safe by ensuring the project's Python requirement covers it (check
requires-python in pyproject.toml is >=3.10) and CI runs a Python >=3.8 (CI uses
3.12); if the project did not guarantee that, replace the expression in the test
that iterates v.components (the zip call with strict=True) with a compatible
alternative (e.g., explicit length check plus plain zip) so tests run on older
interpreters.
Summary
This PR fixes systemic quality gaps in the curriculum while preserving all existing content and code style.
Problems addressed
python-syntaxandtypescript-syntaxadvisory jobsquiz-coverageCI job +scaffold_quizzes.pyscriptpytestCI matrix +scaffold_tests.pyscript + 45 example testspyproject.toml(ruff, mypy, pytest).pre-commit-config.yamlMakefilewith 12 targetsFiles changed (11 files, +1,525 / -8)
New (9):
pyproject.toml— ruff, mypy, pytest config.pre-commit-config.yaml— linting & formatting hooksMakefile— 12 common command targetsscripts/quiz_coverage.py— per-phase quiz gap reportingscripts/scaffold_quizzes.py— quiz.json skeleton generatorscripts/scaffold_tests.py— pytest skeleton generator with AST analysisscripts/ts_syntax_check.mjs— TypeScript syntax checker (no dep resolution needed)phases/01.../tests/test_main.py— 26 linear algebra tests (all pass)phases/14.../tests/test_main.py— 19 agent loop tests (all pass)Modified (2):
.github/workflows/curriculum.yml— 4 new CI jobs (python-syntax, ts-syntax, quiz-coverage, pytest)LESSON_TEMPLATE.md— fixed to match actual lesson structureVerification
Non-destructive
🤖 Generated with Claude Code